-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: external sticker cache #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Type: Enhancement
PR Summary: The pull request introduces an experimental feature that allows external storage caching of stickers for sharing with other applications, such as QQ. It includes changes to the MediaDataController to handle the caching logic and adds a new ConfigCellButton class to manage button cells in the settings activity. Additionally, it modifies the NekoExperimentalSettingsActivity to integrate the new button and its associated actions.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Review the lifecycle management of the new UI components to ensure they are properly cleaned up to prevent memory leaks.
- Consider the appropriate timing and lifecycle events for refreshing the external sticker storage state to ensure it is updated accurately.
- Ensure that the new feature's error handling is robust, especially in the caching process, to prevent application crashes.
- Check for null references in callback methods to avoid null pointer exceptions.
- The PR description could be improved by providing more context on the feature's impact and confirming that it has been tested, as the current description expresses uncertainty about the feature's stability.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
TMessagesProj/src/main/java/tw/nekomimi/nekogram/settings/NekoExperimentalSettingsActivity.java
Outdated
Show resolved
Hide resolved
@@ -133,6 +152,8 @@ public void onItemClick(int id) { | |||
} | |||
}); | |||
|
|||
refreshExternalStickerStorageState(); // Cell (externalStickerCacheRow): Refresh state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): Consider moving the call to refreshExternalStickerStorageState
to a more appropriate lifecycle method if onFragmentCreate
is not the best place to refresh the state, ensuring that the state is refreshed at the correct time.
TMessagesProj/src/main/java/tw/nekomimi/nekogram/settings/NekoExperimentalSettingsActivity.java
Outdated
Show resolved
Hide resolved
TMessagesProj/src/main/java/org/telegram/messenger/MediaDataController.java
Outdated
Show resolved
Hide resolved
TMessagesProj/src/main/java/tw/nekomimi/nekogram/config/cell/ConfigCellButton.java
Outdated
Show resolved
Hide resolved
@ruattd 加一个保存到相册不就行了 |
只是我太懒了想让它自动完成这个过程(外加隔壁 qa 有人写的阴间代码不从相册里面读 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Type: Enhancement
PR Summary: The pull request introduces a new experimental feature that allows caching stickers in external storage for sharing with other applications. It includes changes to various classes to support this functionality, such as adding new menu options for managing the external sticker cache and handling the selection of external directories for the cache.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Review the synchronous calls to
cacheStickers
to ensure they do not adversely affect the main thread's performance. Consider using asynchronous patterns if necessary. - Add error handling for cases where the
ACTION_OPEN_DOCUMENT_TREE
intent cannot be resolved by any app on the device to prevent application crashes. - Clarify the usage of
String()
method ongetExternalStickerCache()
to ensure it is the correct way to check if the external sticker cache configuration is not blank. - Ensure that the new feature is well-documented within the codebase to help future maintainers understand its purpose and implementation.
- Verify that all new user-facing features, such as menu options for managing the external sticker cache, are intuitive and provide a good user experience.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
NaConfig.INSTANCE.getExternalStickerCache().setConfigString(""); | ||
} else { | ||
Intent intent = new Intent(Intent.ACTION_OPEN_DOCUMENT_TREE); | ||
startActivityForResult(intent, INTENT_PICK_EXTERNAL_STICKER_DIRECTORY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): It's good practice to handle the potential ActivityNotFoundException that might be thrown if no app can handle the ACTION_OPEN_DOCUMENT_TREE intent.
@@ -1952,6 +1953,9 @@ public void addNewStickerSet(TLRPC.TL_messages_stickerSet set) { | |||
loadHash[type] = calcStickersHash(stickerSets[type]); | |||
getNotificationCenter().postNotificationName(NotificationCenter.stickersDidLoad, type, true); | |||
loadStickers(type, false, true); | |||
|
|||
// Na: [ExternalStickerCache] cache sticker sets | |||
ExternalStickerCacheHelper.cacheStickers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): Consider the impact of invoking cacheStickers
method synchronously here. If this operation is I/O intensive or time-consuming, it might be beneficial to run it in a separate thread or use an asynchronous approach to avoid blocking the main thread.
@@ -865,6 +869,10 @@ public void requestLayout() { | |||
optionsButton.addSubItem(2, R.drawable.msg_link, LocaleController.getString("CopyLink", R.string.CopyLink)); | |||
optionsButton.addSubItem(3, R.drawable.msg_qrcode, LocaleController.getString("ShareQRCode", R.string.ShareQRCode)); | |||
optionsButton.addSubItem(menu_archive, R.drawable.msg_archive, LocaleController.getString("Archive", R.string.Archive)); | |||
if (!NaConfig.INSTANCE.getExternalStickerCache().String().isBlank()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (llm): The method call String()
on getExternalStickerCache()
seems unusual. Verify that String()
is the correct method to call for checking if the external sticker cache configuration is not blank.
应该没有什么问题了(至少我没测出什么问题来) |
b3af2ab
to
a102cd6
Compare
重新打开以触发 pr check |
Co-authored-by: xtaodada <[email protected]>
Co-authored-by: xtaodada <[email protected]>
实验性功能: 在外部存储缓存贴纸以方便与其他应用共享 (如 QQ 等)
我不确定它会不会炸因为我没法编译 Nagram